Skip to content

Uppercase first letter of diagnostics - #784

Merged
ahoppen merged 1 commit into
swiftlang:mainfrom
ahoppen:ahoppen/uppercase-diag-message
Nov 1, 2023
Merged

ahoppen merged 1 commit into
swiftlang:mainfrom
ahoppen:ahoppen/uppercase-diag-message

Conversation

@ahoppen

@ahoppen ahoppen commented Jul 25, 2023

Copy link
Copy Markdown
Member

sourcekitd returns diagnostics with the first letter lowercase. Xcode, for example, shows the messages with the first letter uppercases. I think that looks nicer and we should also uppercase the first letter in sourcekit-lsp.

CC @tristanlabelle I noticed this in swiftlang/swift#67510. While it doesn’t change the messages to be imperative, having them start with an uppercase letter maybe makes them stick out a little less.

@ahoppen
ahoppen requested a review from benlangmuir as a code owner July 25, 2023 16:53
@ahoppen

ahoppen commented Jul 25, 2023

Copy link
Copy Markdown
Member Author

@swift-ci Please test

@benlangmuir

Copy link
Copy Markdown
Contributor

My concern here is that the first word in the diagnostic might be a keyword or identifier that shouldn't be modified. If this is preferred style, why not update the compiler diagnostics themselves?

@ahoppen
ahoppen force-pushed the ahoppen/uppercase-diag-message branch from 3e3b101 to 7047bc3 Compare July 25, 2023 18:51
@ahoppen

ahoppen commented Jul 25, 2023

Copy link
Copy Markdown
Member Author

@swift-ci Please test

@ahoppen

ahoppen commented Jul 27, 2023

Copy link
Copy Markdown
Member Author

I think on the command line we want to emit diagnostics that start with a lowercase letter to match clang. But in IDEs the errors show up more nicely if they start with an uppercase letter.

If the diagnostic starts with an identifier or keyword, that identifier/keyword should be wrapped in single quotes, which means that the quote would be the first character, so we wouldn’t uppercase the identifier or keyword.

@ahoppen
ahoppen force-pushed the ahoppen/uppercase-diag-message branch from 7047bc3 to 188677f Compare July 27, 2023 22:28
@ahoppen

ahoppen commented Jul 27, 2023

Copy link
Copy Markdown
Member Author

@swift-ci Please test

@ahoppen

ahoppen commented Jul 27, 2023

Copy link
Copy Markdown
Member Author

@swift-ci Please test Windows

1 similar comment
@ahoppen

ahoppen commented Jul 28, 2023

Copy link
Copy Markdown
Member Author

@swift-ci Please test Windows

@bnbarham

Copy link
Copy Markdown
Contributor

I think on the command line we want to emit diagnostics that start with a lowercase letter to match clang. But in IDEs the errors show up more nicely if they start with an uppercase letter.

Do you know if this is actually true? Might be worth bringing up the conversation. FWIW someone actually did the imperative switch: swiftlang/swift#67909 :)

@ahoppen
ahoppen force-pushed the ahoppen/uppercase-diag-message branch from 188677f to d52df40 Compare September 18, 2023 19:49
@ahoppen

ahoppen commented Sep 18, 2023

Copy link
Copy Markdown
Member Author

@swift-ci Please test

@ahoppen
ahoppen force-pushed the ahoppen/uppercase-diag-message branch from d52df40 to 0b911f6 Compare November 1, 2023 00:20
@ahoppen

ahoppen commented Nov 1, 2023

Copy link
Copy Markdown
Member Author

@swift-ci Please test

sourcekitd returns diagnostics with the first letter lowercase. Xcode, for example, shows the messages with the first letter uppercases. I think that looks nicer and we should also uppercase the first letter in sourcekit-lsp.
@ahoppen
ahoppen force-pushed the ahoppen/uppercase-diag-message branch from 0b911f6 to d4f304e Compare November 1, 2023 02:28
@ahoppen

ahoppen commented Nov 1, 2023

Copy link
Copy Markdown
Member Author

@swift-ci Please test

@ahoppen

ahoppen commented Nov 1, 2023

Copy link
Copy Markdown
Member Author

@swift-ci Please test Windows

@ahoppen
ahoppen merged commit 30d1db4 into swiftlang:main Nov 1, 2023
@ahoppen
ahoppen deleted the ahoppen/uppercase-diag-message branch November 1, 2023 04:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants